fix(tui): desynchronize managed reconnect ensure herd#37586
fix(tui): desynchronize managed reconnect ensure herd#37586armancharan wants to merge 34 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Narrow unknown event data with object/`in` checks so sessionID is read without an assertion. Co-authored-by: Cursor <cursoragent@cursor.com>
Helpers only need directory; no test supplies a workspace. Co-authored-by: Cursor <cursoragent@cursor.com>
No test supplies a workspace; avoid an unnecessary brand cast. Co-authored-by: Cursor <cursoragent@cursor.com>
Filter interested subscribers once before encode, require directory for location scope (reject workspace-only queries), and cover those cases in tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Collect matching subscribers first so encode/offer share one pass. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract pure interest helpers and cover subscribeInput/equality plus ClientProvider.event.scope reconnecting only when interest changes. Co-authored-by: Cursor <cursoragent@cursor.com>
Serialize TUI subscribe generations, cancel SSE readers on abort, bridge Bun disconnects so EventFeed queues release, and expose content-free feed counters for leak detection. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid a hard EventFeed.Service requirement on the debug handler so CLI serve Effect inference stays compatible with the runtime handler map. Co-authored-by: Cursor <cursoragent@cursor.com>
Spread ensure calls with pre-ensure jitter, keep attempt-1 ensure-on-first- failure after the spread, and lock concurrent reconnect reuse without spawn. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Replace the hand-rolled unknown guards with decodeUnknownOption so ephemeral sessionID extraction matches host Schema patterns. Co-authored-by: Cursor <cursoragent@cursor.com>
Map HttpApi-decoded ctx.query through interestFromSubscribeQuery, and route deepObject URLSearchParams through the same Schema before building Interest. Co-authored-by: Cursor <cursoragent@cursor.com>
Also derive EventFeed.Diagnostics from the protocol Schema and lock the generation boundary so prior-generation event IDs are not delivered. Co-authored-by: Cursor <cursoragent@cursor.com>
One test was asserting connect, interest change, and no-op scope together. Split into three named cases with a shared mount helper. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Closing this version because the reconnect-herd change is not validated against the missing-service case it is intended to improve. The current tests cover jitter bounds and concurrent reuse of an already-healthy endpoint, but do not measure contender count or recovery latency when several clients ensure a missing service. Recent lifecycle logs also show both single-contender recovery and a slower multi-contender case where a 0-500ms spread would not have covered the observed startup window. We should first add a focused concurrent-ensure benchmark/reproducer, then keep only a policy that measurably reduces launches without delaying normal recovery. |
|
sounds good, thanks for the feedback, @kitlangton! what do you think about something like #38568, for starters, in that case? |
Issue for this PR
Related to #36285
#36285
Parent epic: #36441
Builds on ○ #37585 (stream ownership / post-ensure jitter) and ○ #37570 (ensure-on-first-failure). This PR is the reconnect-herd slice only — update/replacement UX and cold-location cost remain out of scope.
✧ raised · ○ in review · ● approved · ✓ merged
#36441 epic: scope streams + bound payloads
Merge in order:
v2
└ ○ #37486 location interest
└ ○ #37487 session interest + fan-out bench
└ ○ #37585 stream ownership / diagnostics
└ ○ #37586 reconnect herd ← this PR
Siblings under the epic (not git parents of the stack tip):
└ ○ #37559 session blobs + payloadHash
└ ○ #37570 ensure-on-first-failure → feeds #37586
Stacked on #37585 (
event-stream-ownership). GitHub base isv2because the parent branch is fork-only; herd-only diff: armancharan/opencode@event-stream-ownership...reconnect-herd — retarget base onto that branch after #37585 merges.Type of change
What does this PR do?
N TUIs reconnecting after a managed-service restart were calling
Service.ensurein lockstep (attempt 1 skipped backoff), launching manyserve --servicecontenders together.0..500msjitter before everyservice.reconnect()/ ensure so callers desynchronize1000+jitterbackoff when ensure succeedsServerConnection.managedReconnect+ regression tests (aligned with test(cli): lock managed reconnect ensure-on-first-failure #37570) plus a seven-caller concurrent reuse test that must not spawnHow did you verify your code works?
packages/clitest/server-connection.test.ts— ensure-on-first-failure; version reuse; concurrent reconnects reuse one endpoint without spawningpackages/tuitest/cli/tui/reconnect-herd.test.ts— ensure spread / backoff boundspackages/tuitest/cli/tui/use-event.test.tsx— attempt-1 ensure still happens after spread; cleanup aborts in-flight ensurepackages/cliandpackages/tuiAcceptance → test
ensure spread stays within jitter bounds and desynchronizes samples[1000, 1500)post-ensure backoff stays in [1000, 1500)backs off when a resolved event stream keeps failing+managed reconnect ensures the service on the first failureconcurrent managed reconnects reuse one healthy endpoint without spawningmanaged reconnect reuses a healthy service from another versionScreenshots / recordings
N/A — not a UI layout change.
Checklist
Made with Cursor